Skip to content

BUG: fix size()/count() when resamping empty series (#28427) #28459

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

randomstuff
Copy link
Contributor

@randomstuff
Copy link
Contributor Author

I'm not sure this is the best way to fix this.

@WillAyd WillAyd added Resample resample method Datetime Datetime data dtype labels Sep 16, 2019
@WillAyd WillAyd added this to the 1.0 milestone Sep 16, 2019
@randomstuff randomstuff force-pushed the fix_resample_count_empty_series branch from 63ee6bd to ce1a7cc Compare September 16, 2019 18:15
@randomstuff randomstuff force-pushed the fix_resample_count_empty_series branch from ce1a7cc to f3de1d7 Compare September 17, 2019 08:10
Copy link
Contributor

@TomAugspurger TomAugspurger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I'm a bit confused why size and count differ in their return types, but that's for another issue.

@randomstuff randomstuff force-pushed the fix_resample_count_empty_series branch from f3de1d7 to 990dfb4 Compare September 17, 2019 17:55
@randomstuff randomstuff force-pushed the fix_resample_count_empty_series branch from 990dfb4 to e9df4f6 Compare September 17, 2019 22:37
@randomstuff randomstuff force-pushed the fix_resample_count_empty_series branch from e9df4f6 to 5268c86 Compare September 19, 2019 20:27
Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm @jreback

@jreback
Copy link
Contributor

jreback commented Oct 6, 2019

this looked ok, can you merge master and ping on green.

@randomstuff randomstuff requested a review from jreback October 7, 2019 07:27
@randomstuff
Copy link
Contributor Author

@jreback, For some reason, there's still one requested change (which was done). I guess github is expecting yout to validate it?

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comments, ping on green. merge master as well.

@jreback
Copy link
Contributor

jreback commented Oct 8, 2019

@jreback, For some reason, there's still one requested change (which was done). I guess github is expecting yout to validate it?

yes I need to still approve / re-request.

@randomstuff randomstuff force-pushed the fix_resample_count_empty_series branch 2 times, most recently from d0b8b5d to 3df3e17 Compare October 9, 2019 19:12
@randomstuff randomstuff force-pushed the fix_resample_count_empty_series branch 2 times, most recently from bb19e1d to c57db54 Compare October 16, 2019 10:13
@randomstuff randomstuff force-pushed the fix_resample_count_empty_series branch from c57db54 to a928d03 Compare October 16, 2019 12:45
@jreback
Copy link
Contributor

jreback commented Nov 13, 2019

@randomstuff can you merge master and ping on green.

@randomstuff randomstuff force-pushed the fix_resample_count_empty_series branch from a928d03 to f4845eb Compare November 16, 2019 21:42
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code change lgtm. need to use the fixtures for testing. ping on green.

@randomstuff randomstuff force-pushed the fix_resample_count_empty_series branch 2 times, most recently from 1cfdd19 to e4de858 Compare November 19, 2019 08:27
@randomstuff randomstuff force-pushed the fix_resample_count_empty_series branch from e4de858 to aa4e668 Compare November 19, 2019 08:36
@randomstuff
Copy link
Contributor Author

@jreback: all green.

@randomstuff randomstuff requested a review from jreback November 19, 2019 09:30
@jreback jreback merged commit a921092 into pandas-dev:master Nov 19, 2019
@jreback
Copy link
Contributor

jreback commented Nov 19, 2019

thanks @randomstuff

sometimes these things take a while, thanks for sticking with it!

happy to have a followup to consolidate size/count across all grouping types :->

@randomstuff randomstuff deleted the fix_resample_count_empty_series branch November 19, 2019 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Datetime Datetime data dtype Resample resample method
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resampling and counting empty Series does not have a correct dtype
4 participants